feat(navigate): By Coordinates takes DMS, UTM and MGRS - #2002

No actionable comments were generated in the recent review.

Merge Risk: Minimal · up to 68283

The change adds DMS, UTM and MGRS entry to the By Coordinates dialog and map.goToXY, with validation, notices and tests. No outstanding merge-blocking risk was found in the reviewed changes.

Pre-merge checks: 4

Check name	Status	Explanation
Title check		The title names the main change: the By Coordinates dialog and map.goToXY now accept DMS, UTM and MGRS input. The raw summary confirms this as the primary change, and the title uses the feat(navigate)…
Description check		The description follows the repository template. It has Summary, Testing, UI evidence, and Related issue sections. The Summary explains the change and the reason for it. Testing lists the commands run…
Linked Issues check		Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check		Check skipped because no linked issues were found for this pull request.

Full details: Title check

Explanation

The title names the main change: the By Coordinates dialog and map.goToXY now accept DMS, UTM and MGRS input. The raw summary confirms this as the primary change, and the title uses the feat(navigate) scope that matches navigate.rs. Its wording is concise and specific.

Full details: Description check

Explanation

The description follows the repository template. It has Summary, Testing, UI evidence, and Related issue sections. The Summary explains the change and the reason for it. Testing lists the commands run, the new engine and UI tests, and a known failing test on main. UI evidence includes before and after screenshots for a visible dialog change, which the template requires. Related issue names #1042 and #1157. The description covers the required information.

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

_{A rabbit taps a map with care,
And enters coordinates there.
DMS or grids can guide the way,
While scale is checked before the say.
The longitude turns round anew,
Then off the rabbit hops, in view!}

Recent review info

Run configuration

• Source mode: Auto
• Repository: Unavailable
• Branch: Unavailable
• File: No configuration file from this repository applied
• Commit: Unavailable
• Layers: Organization UI
• Review profile: ASSERTIVE
• Plan: Advanced
• Run ID: 25633c89-11ce-4e43-993b-f5f091fab089

Commits

Reviewing files that changed from the base of the PR and between 00d6db4 and 682831a.

Files selected for processing: 3

• crates/engine/src/cmd/search.rs
• crates/engine/src/tests.rs
• crates/ui-egui/tests/smoke.rs

Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 6 remain after this review.

Code review

I found no crash paths, silent data loss, or AGENTS.md violations in the changed lines. The PR has no prior review threads, so nothing was already raised. I read the diff, the f number reader, notify_text, the dialog's open path (it resets all fields with ..Dialogs::default(), so no stale grid text), and parse_lat_lon and axis in search.rs.

Bugs: None found.

Security: None found. Position text is capped at 200 characters and axis text at 64, and the parsers avoid indexing panics.

Performance: None found.

Quality

• Duplicate DMS parsing: read_axis repeats the arithmetic and bounds checks already in search.rs::axis. Posted inline on navigate.rs:461. Medium confidence.
• Notice noise: every unlettered latitude-first pair, including the documented 40.7128, -74.006, gets an "as latitude…, longitude…" notice. The tests cover 2.35, 48.85 and -122.4, 37.8, but nothing pins whether the canonical form should be silent. The first test loop does not check notices. Low-to-medium confidence; this is a UX tradeoff, not a defect.
• Hint text (-74.006 · 74°0′22″W, the grid-reference examples) is not wrapped in tl!. These are examples rather than prose, so this is a nit.

AGENTS.md: No violations found. Notices use notify_text, the interface labels use tl!, and the new command paths are not background work.

Already raised: 0 (no prior threads).

Change size

Compared with main (merge base ce0c6c6 to PR head 3cdd98c), the whole PR.

Code review

Bugs

• Decimal comma silently read as a thousands separator. dialogs.rs:369 strips every comma from the Scale box, so 1,5 becomes 15 (1:15) and the command accepts it with no notice. This is pre-existing logic, but the PR's "refused, not dropped" rule is meant to cover it. Inline comment posted. Confidence: medium.

I found no crash paths: no unwrap/expect/indexing in the new non-test code, and the hostile inputs are bounded (200-char positions, 64-char axes). I also checked the parsing paths for silent approximations: the MGRS square notice is consistent between read_position and read_grid_reference, and the dialog clears text on open, so a stale grid can't grey out the degree boxes.

Security

• None found. The command is effects(NONE), the position text goes through notices as literal data (NoticeText::from(String)), and nothing is read from disk.

Performance

• None found. Parsing is O(input) on bounded input.

Quality (left for the release quality pass, per AGENTS.md; not posted as comments)

• read_axis hard-codes 64 where read_position uses MAX_CHARS. A named constant would read better.
• docs/user-guide/maps.md has an awkward soft wrap between **New Bookmark…** and "saves the current view".

AGENTS.md

• Compliant on the points I checked: errors are reported, the dialog strings use tl!, the new behaviour has both-ways tests, and the screenshots are attached to the PR.

Already raised (left alone): 2

• The tests.rs thread about resetting the map before each axis case. The head now resets before each case (tests.rs around the for (lon, lat) loop and the typographic-quote case), so it looks addressed; the thread is still open, though.
• The navigate.rs thread about duplicated DMS arithmetic. It is outdated: read_axis now calls search::axis.

Change size

Compared: the whole PR against main (merge base dcb1458 to PR head ca90a6e).

📖 Docs preview

GitHub Pages takes a minute or two to publish a new commit.

Code review

Bugs: None confirmed. I traced the DMS and grid parsing paths (read_axis, parse_lat_lon, read_position, go_to_xy, the dialog Go closure). Skipped places are zero-filled, so 74°22″W reads as 74° 0′ 22″, and scale is a string the command's f() accepts. The dialog resets text on open. I did not compile or run the tests, so this rests on reading the code.

Security: None found. Input is length-capped (MAX_CHARS, 64 chars per axis), and the parser uses get/get_mut with no indexing.

Performance: None found. Every parser is linear in input that is already capped.

Quality:

• Low confidence (about 40%), nit: the "lon"/"lat" string path goes through f64::parse, so "lon": "1e1" is accepted as 10°, while position refuses 1e1, 2. The test comment says "1e1 isn't how a latitude is written". Either the text path or the comment should change.
• Medium confidence, design: PositionRead::Ordered fires a notice for every bare pair without letters, including the documented latitude-first form 40.7128, -74.006. That matches the "never silent" rule, but users typing the usual form will see a notice each time. Worth a deliberate decision.
• Low confidence: the engine test loops at lines ~362 and ~383 put { on its own line after a long iterator header. cargo fmt --check may flag that, so check it in CI.

AGENTS.md: No violations found. Checked: no unwrap/expect/indexing in non-test code; the scale refusal and the notices follow "never silent" with tests both ways; new UI strings use tl!; go_to_xy is O(1), so it needs no background path; the docs and screenshots are updated.

Earlier review threads: 4 concerns left alone, none re-posted.

• Open and unchanged: the dialog's replace(',', "") turns a decimal comma in the scale into a thousands separator (1,5 becomes 15). The code still does this, so the earlier thread still applies.
• Addressed: read_axis duplicating search::axis (now calls it, navigate.rs:193).
• Addressed: dropped unit marks on 74°22″W (zero-filling now gives the right value).
• Addressed: test cases reusing the New York center (each now starts from 0, 0).

No inline comments posted: nothing reached the bar for one, and the items above are nits.

Change size

Compared: the whole PR against main (merge base dcb1458 to PR head 9d5c8de).

Code review

Bugs

• None found. I checked the scale path end to end: f() accepts numeric strings, so the dialog's string scale works. scale_text handles decimal commas and the refusal happens before the map is mutated. The coarse-MGRS square notice uses the same parse_mgrs input as read_grid_reference, so the two stay consistent. The dialog resets its fields on open, so stale grid text can't leak in.

Security

• None found. Input lengths are capped (200 characters for positions, 64 for axis values), and I found no panic paths in non-test code.

Performance

• None found. The parsing is bounded and the work is trivial.

Quality

• Misleading message for words ending in N/S/E/W (medium-high confidence, inline comment posted at crates/engine/src/cmd/navigate.rs:420). read_axis checks the first and last characters for hemisphere letters. In the longitude box, Paris (ends in s) gets "S belongs to the latitude", which is wrong. The test at crates/engine/src/tests.rs:45880 passes anyway because the message also contains "write the longitude".
• Notice on every bare-number pair (low confidence, nit). Any unlettered position pair such as 40.7128, -74.006 gets an "as latitude … longitude …" notice, even when it is in the usual order. This follows the PR's stated design, so it may be intentional. If it is not, the notice could be limited to pairs where the order was actually swapped.

AGENTS.md

• No violations found. The new strings use tl!, there are no unwrap/expect calls in non-test code, no new dependencies, and no background-command or wasm concerns. map.goToXY now refuses an unusable scale rather than dropping it. map.camera forwards its scale to go_to_xy, so it inherits the refusal. The PR body doesn't mention this, and it may be worth a line in the docs.

Earlier review threads

• Four earlier threads were left alone: the duplicated DMS arithmetic, the decimal comma in the scale box, the test that didn't reset the map, and the dropped unit marks. The current code addresses all four, so none needs to be raised again. None is still open.

Change size

Compared with the base branch (merge base dcb1458 to PR head 09e47cd).

Claude autofix

• Merged main and resolved conflicts in 2 file(s).
• Review threads: 2 addressed, 1 fixed.

Code review

I read the full diff, the staged metadata and prior threads, and the surrounding code in search.rs, dialogs.rs, mapmenu.rs and lib.rs. I could not build or run the tests here (no shell), so the findings below come from reading the code.

Bugs: No new bugs found. I traced read_axis's hemisphere, mark and ordering logic (40°30″, 40 30″, 74°22″W, 40 30°, duplicate and out-of-order marks, 40.5°30′) and the scale path (f() filters non-finite values, so inf/NaN strings are refused). Dialog state resets on open, so a stale bookmark name can't reach the grid box. Confidence: high.

Security: None found. Input is bounded (200 characters for position, 64 for an axis box), the parsers index with get/get_mut, and the non-test code has no unwrap, expect or unsafe. Confidence: high.

Performance: None found. Parsing is O(input length) on bounded input, and goToXY is instant, so no background path is needed. Confidence: high.

Quality (nits, grouped):

• scale_text in dialogs.rs refuses a leading + with grouping (+50,000) while +50000 is accepted. Trivial. Confidence: medium.
• Any lat-first decimal pair such as 40.7128, -74.006 gets a "Read … as latitude …" notice, because both values are within ±90 and the order is a real assumption. This matches the doc comment's intent, but the notice fires on the most common form. Worth confirming that is what you want. Confidence: medium.
• Centre exactly on 180° prefills as -180.0000. Same meridian, but the displayed value changes. Confidence: low.

AGENTS.md: Nothing found. Notices are given for the lossy readings (square centre, bare-number order) and the refusals (scale, grid references) with a test both ways. Engine errors stay English, as notice_text.rs allows. Dialog strings use tl!. The new doc text is in maps.md, and before/after screenshots are in the PR body. Confidence: high.

Earlier threads: I left 8 alone. Six are resolved and their fixes are in the code: the Paris hemisphere message, read_axis sharing search::axis, the scale comma, the 74°22″W marks, the test reset, and the antimeridian prefill. Two are still open and still valid, and I did not re-post them:

• navigate.rs ~346: fractional degrees with minutes (40.5°30′N) read as 41° with no notice. read_axis refuses this, but parse_lat_lon → axis (search.rs:723) does not, and the position path reaches it silently.
• smoke.rs ~4659: the grid-reference assertion passes even if grid navigation does nothing, because the map is already at New York from the previous step.

Change size

Compared with main (merge base eef1a9d to PR head 00d6db4). Sizes are file sizes, not line counts.

Code review

I read the full diff, the base-branch AGENTS.md, the prior review threads and the helpers the new code calls (search::axis, parse_lat_lon, read_grid_reference, the notice API, the dialog open path). I could not compile or run the tests in this environment, so this review is by reading only. The author reports cargo xtask ci passing apart from one unrelated test.

Bugs: None found. I traced the read_axis mark placement (°, ′, ″, typographic quotes, gaps, out-of-order marks), the hemisphere-letter handling, the scale and grid paths, and the dialog open path. The dialog's Dialogs::default() reset means a stale bookmark name can't reach the grid box, and every failure returns before the camera moves.

Security: None found. Inputs are length-capped (200 characters for positions, 64 for axes), there are no unwrap/expect calls in non-test code, and every index goes through get/get_mut.

Performance: Nothing to raise. Inputs are tiny and the work is constant.

Quality:

• (confidence ~0.6, inline comment posted) search::parse_lat_lon accepts -40 N and reads it as -40 without a notice (search.rs:728). read_axis refuses the same contradiction, so the position path and the dialog now disagree.
• (low confidence, not posted) The shared search::axis change means the search box now refuses fractional-degree DMS such as 40.5°30′N, where it previously read them as 41°. That fixes a wrong reading, but the PR description does not mention the search-box change, and no search-box test covers the refusal.
• (nits, not posted) The maps.md edit leaves **New Bookmark…** on a line of its own inside the paragraph. The dialog's wrap_lon prefill shows -180.0000 at exactly 180°, which is the same place on the globe.

AGENTS.md: The changed code follows the repository rules I checked: no dialogs in commands, UI strings in tl!, and refusals rather than silent drops for the scale and the wrong-axis letter. The new engine and UI tests cover both the lossy and clean cases. The one gap is the search-box behaviour change above, which the AGENTS.md rule on silent changes would expect to be disclosed.

Prior reviews: I left 7 earlier concerns alone because they were already raised. Six are resolved or addressed in code. The seventh, the fractional-degrees thread on navigate.rs:346, is still open, but the current axis check at search.rs:653 covers every parse_lat_lon path, so it can probably be resolved.

Change size

Compared with the base branch (the whole PR, merge base eef1a9d to PR head 682831a).

The failing "Lint (pre-commit)" check comes from main, not this PR: crates/engine/src/box_edit.rs:365 and crates/ui-egui/src/boxedit.rs:359 (from #1959) have a bare #[allow(clippy::unwrap_used, clippy::expect_used)], which #2063's lint rule now refuses. Main's own CI fails the same check at 8d99782.

@coderabbitai review